-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify the drake_cmake_external example #338
Simplify the drake_cmake_external example #338
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+@nicolecheetham for review please
Reviewable status: all discussions resolved, LGTM missing from assignee nicolecheetham, platform LGTM missing (waiting on @nicolecheetham)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee nicolecheetham, platform LGTM missing (waiting on @BetsyMcPhail)
drake_cmake_external/drake_external_examples/CMakeLists.txt
line 18 at r1 (raw file):
get_filename_component(PYTHONPATH "${drake_DIR}/../../python${PYTHON_VERSION_MAJOR}.${PYTHON_VERSION_MINOR}/site-packages"
The Jenkins job failed. It seems one of the import_all_test.py tests was not able to run as it was unable to find the executable. I think keeping the get_filename_component command should resolve this. If you know another way that is fine too.
drake_cmake_external/drake_external_examples/CMakeLists.txt
line 6 at r1 (raw file):
project(drake_external_examples) find_package(Python3 REQUIRED COMPONENTS Interpreter Development)
I believe after Python3 you would need to put ${drake_PYTHON_VERSION} EXACT like that of the drake_cmake_installed CML.
7b84188
to
f0c2001
Compare
Updates to the CMakeLists include: * Use the modern method of finding Python. * Remove unnecessary subdirectory
f0c2001
to
914b643
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicolecheetham this is ready for another look
Reviewable status: 2 unresolved discussions, LGTM missing from assignee nicolecheetham, platform LGTM missing (waiting on @nicolecheetham)
drake_cmake_external/drake_external_examples/CMakeLists.txt
line 6 at r1 (raw file):
Previously, nicolecheetham (Nicole C.) wrote…
I believe after Python3 you would need to put ${drake_PYTHON_VERSION} EXACT like that of the drake_cmake_installed CML.
Done.
drake_cmake_external/drake_external_examples/CMakeLists.txt
line 18 at r1 (raw file):
Previously, nicolecheetham (Nicole C.) wrote…
The Jenkins job failed. It seems one of the import_all_test.py tests was not able to run as it was unable to find the executable. I think keeping the get_filename_component command should resolve this. If you know another way that is fine too.
I had failed to update some Python-related variables, it is passing now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all discussions resolved, platform LGTM missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+@rpoyner-tri for platform review please
Reviewable status: all discussions resolved, LGTM missing from assignee rpoyner-tri, platform LGTM missing (waiting on @nicolecheetham and @rpoyner-tri)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 4 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: complete! all discussions resolved, platform LGTM from [rpoyner-tri] (waiting on @BetsyMcPhail)
Updates to the CMakeLists include:
This PR splits out the non-setup changes from #313, working towards #61
This change is